Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the ability to reflect Inductive definitions #32

Merged
merged 20 commits into from
Dec 8, 2016

Conversation

aa755
Copy link

@aa755 aa755 commented Dec 1, 2016

This is implemented as the command Make Inductive.
See demo.v for a demo.

aa755 added 15 commits November 14, 2016 23:33
Just created a new vernac command "Make Inductive".
As of now, it does nothing. Tested the dummy command in demo.v
The body is ignored for now. Tested this part in demo.v
The name and arity are obtained properly populated. However the
resultant definition changes arity from Set to Prop.
Found the correct input by snooping on the paramcoq plugin (using
coq/dev/top_printer.ml)
Also, needed to recompile coq so that top_printer.ml gets included in
coqtop/coqc:

coq/dev$ echo "Top_printer" > printer.mllib
then add dev/printer.cma to $CORECMA in coq/Makefile.common
@gmalecha
Copy link
Owner

gmalecha commented Dec 2, 2016

Thanks for the PR. This is a great feature to have.

Is there a reason that you chose not to use the existing quoting of inductive types? In particular (https://github.com/gmalecha/template-coq/blob/master/theories/Ast.v#L63) Does it not contain enough information? My worry with exposing such low level types is two fold:

  1. They don't fit naturally into the mental model that users have (maybe that is the wrong mental model?)
  2. They are difficult (verbose?) to program with.

@aa755
Copy link
Author

aa755 commented Dec 2, 2016

That information is not sufficient, as pointed out by Jason:
#17

All the fields in Ast.mutual_inductive_entry and Ast.one_inductive_entry (except maybe mind_entry_private) seem semantically relevant to me.

@gmalecha
Copy link
Owner

gmalecha commented Dec 2, 2016

I don't doubt that they are semantically relevant, it just seems like it is a representation that is not natural to Coq users.

To address the problem that @JasonGross pointed out, would it be sufficient to add a type to the inductive as a whole? Something like:

| PType : ident -> term (* the type *)-> list (ident * inductive_body) -> program -> program

My worry is that everyone that uses this library is going to have to reverse engineer what this type does. A better solution (if it can be made to work) would be to have something that maps more directly to how Coq users write inductive types.

@aa755
Copy link
Author

aa755 commented Dec 2, 2016

I did have some difficulty in figuring out the de-bruijn indices for the types of constructors.
I may sometime write in Gallina a high-level wrapper to compute the argument to Make Inductive, perhaps from something that is named (instead of de-bruijn) and looks close to how Coq users write inductives. However, I would prefer to keep the OCaml part (in reify.ml4) as simple as possible.

Your suggested representation still loses information that is relevant to me (parameters vs indices, flags for template/universe polymorphism).

@JasonGross
Copy link
Contributor

I believe mind_entry_private is about whether or not it's okay to pattern match on the inductive outside the module it's defined in; this is to allow Licata's HIT trick.

Copy link
Collaborator

@mattam82 mattam82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @gmalecha that it would be nicer to find a way to merge this info in PType somehow, otherwise we have a strange redundancy. Currently template-coq has this "program" datatypes that does not correspond to something in Coq. Why not switch to an env + term representation here?
Then the inductive decls and global definitions would be in the env. We could make a compatibility layer that would produce a [program] from those.



Definition local_entry : Set := term.
(*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not LocalDef and LocalAssum here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does LocalDef stand for local let bindings?


Record mutual_inductive_entry : Set := {
mind_entry_record : option (option ident);
(* mind_entry_finite : Decl_kinds.recursivity_kind; (* inductive/coinductive/record*)*)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the finiteness information can be useful (ind vs coind in particular)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I was being lazy.

mind_entry_params : list (ident * local_entry);
mind_entry_inds : list one_inductive_entry;
mind_entry_polymorphic : bool;
(* mind_entry_universes : Univ.universe_context; (*what is this?*) *)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the universe context of the inductive: either global universes and constraints that must be declared or local in case of universe polymorphism (it records the universe binders of all the inductives/constructors in that case). This is not handled by template-coq anywhere currently.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a lot of issues trying to get this to work due to the way that Coq represents universes. Issue #1 is about universes. If you have any thoughts on how to reify and reflect universes, I think that is the biggest problem with template-coq right now.

mind_entry_inds : list one_inductive_entry;
mind_entry_polymorphic : bool;
(* mind_entry_universes : Univ.universe_context; (*what is this?*) *)
mind_entry_private : option bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed private is for the HIT trick

@gmalecha
Copy link
Owner

gmalecha commented Dec 2, 2016

I think then env + term representation would be a good route to go here. At minimum this would mean pulling the constructors of program out and make them the environment values.

@mattam82 I got confused about the last comment in your comment. Do you think that we should convert these new definitions to something like PType in Gallina, or in the plugin?

@mattam82
Copy link
Collaborator

mattam82 commented Dec 2, 2016

@gmalecha in Gallina, as part of the plugin's v files we could have an env + term -> program function to get back the closed representation.

@mattam82
Copy link
Collaborator

mattam82 commented Dec 2, 2016 via email

@gmalecha
Copy link
Owner

gmalecha commented Dec 4, 2016

@aa755 Why don't we export the entire type for this PR, and then open a second issue and PR to add the environment representation that @mattam82 suggested. That will let us merge this relatively quickly and it won't break anything for users.

mind_entry_private := None;
|}.

(* Make Inductive ltac:(let t:= eval compute in mut_i in exact t). *)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this perform reduction so that we don't need to use tactics in terms to make it work? I don't know precisely how to do this, but I'm certain that @mattam82 could tell us in a minute.

| Finite
| CoFinite
| BiFinite
*)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an out-of-place comment.

| LocalDef of constr
| LocalAssum of constr
*)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(** The kind of definition. See kerne/....mli **)

type local_entry =
| LocalDef of constr
| LocalAssum of constr
*)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the next comment.

VERNAC COMMAND EXTEND Unquote_inductive CLASSIFIED AS SIDEFF
| [ "Make" "Inductive" constr(def) ] ->
[ check_inside_section () ;
let (evm,env) = Lemmas.get_current_context () in
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to check_inside_section here?

@@ -766,6 +826,14 @@ VERNAC COMMAND EXTEND Unquote_vernac CLASSIFIED AS SIDEFF
[] None result None (Lemmas.mk_hook (fun _ _ -> ())) ]
END;;

(* get rid of the unused ident(name)? *)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

@gmalecha gmalecha merged commit db3da94 into gmalecha:coq-8.5 Dec 8, 2016
@gmalecha
Copy link
Owner

gmalecha commented Dec 8, 2016

Merging!

gmalecha pushed a commit that referenced this pull request Jun 23, 2018
Rename Constraint to ConstraintSet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants